--[BE][WIP] Remove Eigen dependency #2301

jturner65 · 2024-01-15T20:32:33Z

Motivation and Context

This PR removes Eigen library/dependency from Habitat-sim. It is intended to be an invisible change, and may require iterations to get everything.

Still a WIP.

How Has This Been Tested

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

mosra

Hasty review while waiting for the CI.

You might want to clean up remains of Eigen in dependencies.cmake afterwards as well, including not enabling the EigenIntegration library anymore.

src/esp/core/Utility.h

src/esp/geo/CoordinateFrame.h

mosra · 2024-01-16T20:44:08Z

src/esp/geo/Geo.cpp

@@ -18,20 +20,21 @@ using Magnum::Math::Literals::operator""_rgb;
 namespace esp {
 namespace geo {

-std::vector<vec2f> convexHull2D(const std::vector<vec2f>& points) {
+std::vector<Mn::Vector2> convexHull2D(const std::vector<Mn::Vector2>& points) {


Could take a const array view while you're at it.

src/esp/geo/OBB.cpp

src/esp/geo/OBB.h

src/utils/datatool/Mp3dInstanceMeshData.cpp

src/utils/datatool/Mp3dInstanceMeshData.h

src/utils/datatool/Mp3dInstanceMeshData.cpp

src/utils/datatool/Mp3dInstanceMeshData.h

mosra · 2024-01-16T20:57:58Z

src/utils/datatool/SceneLoader.cpp

+        const Mn::Vector3 xyz_scene =
+            Mn::Vector3::from(&assimpMesh.mVertices[v].x);
+        const Mn::Vector3 xyz_esp =
+            alignSceneToEspGravity.transformVectorNormalized(xyz_scene);
        mesh.vbo.push_back(xyz_esp);

        if (assimpMesh.mNormals) {


Um, there's a whole AssimpImporter for this kind of thing. Can't the datatool be just deleted or something?

A cleaner and safer way (bounds checking, type compatiblity checking...) instead of Mn::Vector3::from() etc would be to take the whole mNormals array, make an array view and cast it:

Cr::Containers::ArrayView<const Mn::Vector3> normals = Cr::Containers::arrayCast<const Mn::Vector3>(Cr::Containers::arrayView(assimpMesh.mNormals, assimpMesh.mNumVertices));

And similar for positions etc.

Yeah, I know, I just don't want to make that decision unilaterally. @aclegg3 Thoughts?

mosra · 2024-01-23T10:44:32Z

src/esp/core/EspEigen.h

+// template <typename T>
+// std::ostream& operator<<(std::ostream& os, const Map<T>& m) {
+//   return os << m.format(kJsonFormat);
+// }


Not sure what you plan here, but one way to efficiently format these might be with Utility::JsonWriter array support and vector.data() / matrix.data(), which both return a statically sized array reference.

mosra · 2024-01-23T10:45:10Z

src/esp/core/Utility.h

+             Magnum::Math::cross(fromNorm, halfVec).normalized(),
+             Magnum::Math::dot(fromNorm, halfVec))
+      .normalized();
+}  // quatRotFromTwoVectors


Probably too much to ask in this context, but I'd like to have this contributed to Magnum :)

src/esp/geo/CoordinateFrame.cpp

src/esp/geo/CoordinateFrame.h

mosra · 2024-01-23T10:51:21Z

src/esp/nav/PathFinder.cpp

@@ -988,7 +995,7 @@ struct NavMeshTileHeader {
 };

 struct Triangle {
-  std::vector<vec3f> v;
+  std::vector<Mn::Vector3> v;
  Triangle() { v.resize(3); }
 };


Oh, wow, ew, can't this be just a Mn::Vector3 v[3]? :D Or a Containers::StaticArray if you want bounds check in debug.

src/esp/sensor/Sensor.cpp

src/tests/GeoTest.cpp

mosra · 2024-01-23T11:15:57Z

src/utils/viewer/viewer.cpp

+      auto v = agentBodyNode_->translation();
+      Cr::Utility::formatInto(res, res.length(),
+                              "Agent position  : [{} {} {}] ", v.x(), v.y(),
+                              v.z());


Unnecessarily overcomplicated, just use the builtin debug output ~~redirected to a stringstream~~. The redirection and if(res.size() > 0) is also redundant, if it's going to get printed it's non-empty already anyway.

ESP_DEBUG() << "Agent position:" << agentBodyNode_->translation();

Same below. If you want a more compact output for vectors (and quats), use Mn::Debug::packed << agentBodyNode_->translation() instead.

src/utils/viewer/viewer.cpp

mosra · 2024-01-27T20:21:45Z

src/esp/nav/PathFinder.cpp

  const int numVerts = mesh.vbo.size();
  const int numIndices = mesh.ibo.size();
  const float mf = std::numeric_limits<float>::max();
-  vec3f bmin(mf, mf, mf);
-  vec3f bmax(-mf, -mf, -mf);
+  Mn::Vector3 bmin(mf, mf, mf);
+  Mn::Vector3 bmax(-mf, -mf, -mf);

  for (int i = 0; i < numVerts; ++i) {
-    const vec3f& p = mesh.vbo[i];
-    bmin = bmin.cwiseMin(p);
-    bmax = bmax.cwiseMax(p);
+    const Mn::Vector3& p = mesh.vbo[i];
+    bmin = Mn::Math::min(bmin, p);
+    bmax = Mn::Math::max(bmax, p);
  }


Was working on something unrelated and came across MeshTools::boundingRange(), which is basically just a call to Math::minmax(). No fancy SIMD internals there yet, but it's a candidate.

rebase error

…trices

With the Corrade update, no need to use Matrix3x3

magnum points can be directly compared

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jan 15, 2024

jturner65 requested a review from aclegg3 January 15, 2024 20:32

jturner65 force-pushed the BE_RemoveEigenDeps branch 6 times, most recently from 78eee5f to 76938f1 Compare January 16, 2024 17:57

mosra reviewed Jan 16, 2024

View reviewed changes

jturner65 force-pushed the BE_RemoveEigenDeps branch 4 times, most recently from fdb649c to 3f6882e Compare January 22, 2024 21:14

mosra reviewed Jan 23, 2024

View reviewed changes

jturner65 force-pushed the BE_RemoveEigenDeps branch 2 times, most recently from 4aa0dff to 2f43fa1 Compare January 25, 2024 15:11

mosra reviewed Jan 27, 2024

View reviewed changes

mosra mentioned this pull request Jan 29, 2024

Update Magnum and m.css to latest #2309

Merged

jturner65 force-pushed the BE_RemoveEigenDeps branch 3 times, most recently from afae3b5 to 1507e23 Compare February 5, 2024 13:58

jturner65 force-pushed the BE_RemoveEigenDeps branch 2 times, most recently from e382b1c to b371c7b Compare February 13, 2024 14:12

jturner65 force-pushed the BE_RemoveEigenDeps branch 2 times, most recently from 4d28775 to aef35d7 Compare February 21, 2024 14:32

jturner65 force-pushed the BE_RemoveEigenDeps branch 2 times, most recently from 7b25699 to f773e28 Compare February 29, 2024 17:05

jturner65 force-pushed the BE_RemoveEigenDeps branch 3 times, most recently from 49557ad to d58931c Compare March 5, 2024 20:02

jturner65 added 28 commits October 29, 2024 17:06

--fix objectControls

85ce0aa

--fix agent

f5ed667

--more cleanup.

29c663a

--bugfixes

22df61f

--bug fixes

81ecb70

--more bugfixes

09f8074

--still more bugfixes

5210eba

--oops

96924d8

--reviewer suggestions

78c1224

--clang-tidy

88843f3

--reviewer comments

587ef38

--missing include

a1f09a0

--reviewer suggestions

98a4f31

--fix an error - shouldn't normalize the cross product.

d5f0d7e

--cleanup;

a3cdec4

--use join function

6d20c4e

--fix bbox bindings issue

eba9bb5

--fix broken constructor

9b81af6

rebase error

--another ortho test mistake

b6f62ed

--replace quat rotation from 2 vectors function with magnum version

dae7330

--Specify rotations as Matrix3x3 instead of full 2D transformation ma…

935edaa

…trices

--cleanup

d7a39b7

--missing include

534b059

--cleanup

4cc3396

--return to Matrix3

ba67781

With the Corrade update, no need to use Matrix3x3

--remove call to obviated function

75495eb

--remove unused include

5d97011

--cleanup

1ba7972

jturner65 force-pushed the BE_RemoveEigenDeps branch from 7346b42 to 1ba7972 Compare October 29, 2024 21:07

--fix test

80e5ead

magnum points can be directly compared

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--[BE][WIP] Remove Eigen dependency #2301

--[BE][WIP] Remove Eigen dependency #2301

jturner65 commented Jan 15, 2024 •

edited by aclegg3

Loading

mosra left a comment

mosra Jan 16, 2024

mosra Jan 16, 2024

jturner65 Jan 16, 2024

mosra Jan 23, 2024

mosra Jan 23, 2024

mosra Jan 23, 2024

mosra Jan 23, 2024

mosra Jan 27, 2024 •

edited

Loading

--[BE][WIP] Remove Eigen dependency #2301

Are you sure you want to change the base?

--[BE][WIP] Remove Eigen dependency #2301

Conversation

jturner65 commented Jan 15, 2024 • edited by aclegg3 Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

mosra left a comment

Choose a reason for hiding this comment

mosra Jan 16, 2024

Choose a reason for hiding this comment

mosra Jan 16, 2024

Choose a reason for hiding this comment

jturner65 Jan 16, 2024

Choose a reason for hiding this comment

mosra Jan 23, 2024

Choose a reason for hiding this comment

mosra Jan 23, 2024

Choose a reason for hiding this comment

mosra Jan 23, 2024

Choose a reason for hiding this comment

mosra Jan 23, 2024

Choose a reason for hiding this comment

mosra Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

jturner65 commented Jan 15, 2024 •

edited by aclegg3

Loading

mosra Jan 27, 2024 •

edited

Loading